Enhance multiple_outputs inference of dict typing
#19608
Conversation
github-actions
bot
added
the
full tests needed
|
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
josh-fell
force-pushed
the
taskflow-api-multiple-output-inference
branch
from
656e595
to
fdaf89a
Compare
josh-fell
force-pushed
the
taskflow-api-multiple-output-inference
branch
from
fdaf89a
to
b92f49a
Compare
josh-fell
force-pushed
the
taskflow-api-multiple-output-inference
branch
2 times, most recently
from
341740c
to
3cc024d
Compare
josh-fell
force-pushed
the
taskflow-api-multiple-output-inference
branch
from
3cc024d
to
a7a6df1
Compare
|
@uranusjr I noticed in another PR that there were other references to tuples and lists in |
|
Looks like a new static check was added in #20501 which checks syntax for 3.7+ and is attempting to remove the check for Python 3.6 in the typing inference logic here. Is dropping Python 3.6 support slated for 2.2.4? Or should I go ahead and remove the Python 3.6 check and assume it's no longer supported even though #20467 is not part of a release yet? |
Interesting one that we have not foreseen. I think we should disable the pyupgrade for now and re-enable it when we release 2.3.0. |
|
@potiuk Would you like me to open a PR to disable the pyupgrade check or were there other conversations on handling this edge case until the 2.3 release? |
| @task_decorator | ||
| def identity_dict(x: int, y: int) -> eval(test_return_annotation): | ||
| return {"x": x, "y": y} |
There was a problem hiding this comment.
Would it be possible/easy to also handle the stringified annotation, e.g.
@task_decorator
def identity_dict(x: int, y: int) -> "dict":
return {}There was a problem hiding this comment.
Yeah I don't see why not. I'll dig in.
There was a problem hiding this comment.
Looks like typing.get_type_hints() does the eval automatically and can be useful here (to replace inspect.signature).
There was a problem hiding this comment.
From what I can tell we eventually get into the same __origin__ vs __extra__ attribute situation for Python 3.6 unfortunately. We can definitely add some tests for the stringified dict typing though.
Also looks like the recent refactor of airflow/decorators/base.py uses inspect.signature a little more extensively too.
There was a problem hiding this comment.
Python 3.6
Haven’t we dropped it already?
There was a problem hiding this comment.
Technically it won't be dropped until 2.3 but this PR is slated for 2.2.4.
There was a problem hiding this comment.
Would it be viable to do de-stringification if 3.6 is not considered? We can add this in a separate PR that targets only 2.3+ if so.
Disabling - no but changing it back to 3.6 syntax (you can do it in this PR) with a note that it should be bumed to 3.7 when we release 2.3.0 |
josh-fell
force-pushed
the
taskflow-api-multiple-output-inference
branch
from
0b036ea
to
d7378a9
Compare
(cherry picked from commit 4198550)
Closes: #19538
There are a few cases in which the existing
multiple_outputsinference in the TaskFlow API doesn't automatically unfurl a returned dictionary into separateXComs:dicttypemultiple_outputsto True due to use of an incorrect attrThis PR fixes the inference on a simple
dictreturn type annotation and adds a condition for Python 3.6 use.^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.